Skip to content
This repository has been archived by the owner on Sep 20, 2023. It is now read-only.

Get kms key from env #4

Merged
merged 6 commits into from
Dec 3, 2022
Merged

Get kms key from env #4

merged 6 commits into from
Dec 3, 2022

Conversation

magreenbaum
Copy link
Contributor

@magreenbaum magreenbaum commented Dec 2, 2022

The KMS key ID is now passed to the container via an env var KMS_KEY_ID automatically so we don't need to provide kms_key_id when encrypting anymore.

Decrypting should retrieve this value from the environment.

Closes: #3

kms_ext/extension.py Outdated Show resolved Hide resolved
Copy link
Member

@WillDaSilva WillDaSilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nice quality-of-life change would be to take the KMS key ID as a command-line argument. Typer/Click already support setting optional CLI arguments via env vars. Then we could raise an exception if it's not provided as we currently do.

That said, what we have here is good enough until we switch away from kms-ext. Users won't be doing any decryption themselves with it after all, unless it is used for things other than Meltano Cloud.

Copy link

@aaronsteers aaronsteers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this change, actually. Is there a good reason to not put the key into the secrets file? (For context, we're having the same discussion over on the encrypted config spec.)

I want to make sure we have clear erroring when keys are rotated - and also that we leave the door open to the same container potentially having 2 or more available keys at one time. Can we instead do both - put the key ID in the file so it is explicit rather than implicit - and throw an error (or at least a warning) if the key that is mentioned in the file is not the same Key ID as the one passed in the env var?

@magreenbaum
Copy link
Contributor Author

I'm not sure about this change, actually. Is there a good reason to not put the key into the secrets file? (For context, we're having the same discussion over on the encrypted config spec.)

I want to make sure we have clear erroring when keys are rotated - and also that we leave the door open to the same container potentially having 2 or more available keys at one time. Can we instead do both - put the key ID in the file so it is explicit rather than implicit - and throw an error (or at least a warning) if the key that is mentioned in the file is not the same Key ID as the one passed in the env var?

My initial thoughts around this was to simplify the encryption process (not requiring a key id). But with expectations of key rotation or multiple kms keys as an option in the future, probably worth keeping in that case. I'll refactor based on this.

@aaronsteers
Copy link

aaronsteers commented Dec 2, 2022

My initial thoughts around this was to simplify the encryption process (not requiring a key id).

Oh, got it! Yeah, that makes sense. 👍

We can still get the KMS Key ID from env var while encrypting and then also automatically store it in the yaml, I think that's a nice balance. Edit: Per the below: we want encryption to work only with the public key, without the user needing to receive or track their KMS Key.

@WillDaSilva
Copy link
Member

We can still get the KMS Key ID from env var and then also automatically store it in the yaml, I think that's a nice balance.

@aaronsteers If we want to store it in the yaml, we need it to be provided during the encrypt command. In that case, I don't see why we'd want to force it to be provided via env var.

Providing it via env var is useful for the decrypt command because it's convenient for us to inject env vars into the runner.

But don't we plan to move away from this approach towards secrets relatively soon anyway? I don't think we'll ever get around to supporting key rotation with kms-ext since by the time we can prioritize that, we'll hopefully already be using a secrets backend powered by meltano login and meltano config --secret.

@aaronsteers
Copy link

aaronsteers commented Dec 2, 2022

We can still get the KMS Key ID from env var and then also automatically store it in the yaml, I think that's a nice balance.

@aaronsteers If we want to store it in the yaml, we need it to be provided during the encrypt command.

Yes! Thank you. I was switching context from the other issue, where there is a known explicit key at encrypt time and any number of potential decrypt keys at decrypt time. This is also true here except we're starting from the public key and not from a Key ID. Okay, I'm caught up now. Presumably the closest 'ID' we have at encrypt time is a public key fingerprint, yes? Because we don't want the user to have to concern themselves with the Key ID and also the public key while encrypting - which makes sense.

I will reread with the improved context and reply again shortly. Thanks, both.

@aaronsteers
Copy link

aaronsteers commented Dec 2, 2022

Logged for follow-up:

It's probably not a ton of work to add a fingerprint (MD5 or SHA-based hash of the key text), but it isn't strictly needed as of now.

But don't we plan to move away from this approach towards secrets relatively soon anyway?

That's an important question, and it does play into my thinking here. I don't expect this will fully go away with meltano/meltano#6999.

Rather than slow down this PR, I've put some thoughts into a new discussion here in this repo:

Copy link

@aaronsteers aaronsteers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for the discussion here. I'm happy for this to merge as it is. 👍

@magreenbaum magreenbaum merged commit 36b4438 into main Dec 3, 2022
@magreenbaum magreenbaum deleted the get_kms_key_from_env branch December 3, 2022 13:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor utility to get kms key id from an env var
3 participants